Explain API changes #2403
Explain API changes #2403
Conversation
neetikasinghal
force-pushed
the
explain
branch
from
93c1f96 to
8b52ed7
Compare
There was a problem hiding this comment.
We need to have Unit test for each case
- Disk-based search with valid rescore context.
- Radial search.
- ANN search (default case).
- Shard-level rescoring enabled.
- Shard-level rescoring disabled.
- Filter weight case where filtered IDs are less than k.
- Filter threshold value greater than cardinality.
- Missing native engine files.
- Valid context with matching document and disk-based search.
And Please validate with Explaination Object.
@Vikasht34 yes the tests are not yet added in here, hence its in a draft status. I will add the coverage with all the possible cases. |
neetikasinghal
force-pushed
the
explain
branch
from
8b52ed7 to
bffbdac
Compare
Signed-off-by: Neetika Singhal <[email protected]>
neetikasinghal
force-pushed
the
explain
branch
from
bffbdac to
7c4f425
Compare
neetikasinghal
requested review from
heemin32,
navneet1v,
VijayanB,
vamshin,
jmazanec15,
naveentatikonda,
junqiu-lei,
martin-gaievski,
ryanbogan,
luyuncheng,
shatejas and
0ctopus13prime
as code owners
|
@navneet1v / @Vikasht34 would you please review the changes? |
|
@neetikasinghal can you please add an entry in the change log |
yup i generally add it towards the end of the review so that its easier to rebase with the latest changes. |
| @@ -46,6 +51,11 @@ public float scoreTranslation(float rawScore) { | |||
| return 1 / (1 + rawScore); | |||
| } | |||
|
|
|||
| @Override | |||
| public String explainScoreTranslation(float rawScore) { | |||
| return "`1 / (1 + rawScore)`"; | |||
There was a problem hiding this comment.
can we make this as private static final string.
| @@ -77,6 +87,11 @@ public float scoreTranslation(float rawScore) { | |||
| return Math.max((2.0F - rawScore) / 2.0F, 0.0F); | |||
| } | |||
|
|
|||
| @Override | |||
| public String explainScoreTranslation(float rawScore) { | |||
| return "`Math.max((2.0F - rawScore) / 2.0F, 0.0F)`"; | |||
| if (score == 0) { | ||
| score = knnScore; | ||
| } |
There was a problem hiding this comment.
shouldn't this be the first check we do when we enter the if condition.
There was a problem hiding this comment.
not really, knnScore should be computed for it to be assigned to the score.
| } | ||
|
|
||
| private float getKnnScore(LeafReaderContext context, int doc) throws IOException { | ||
| KNNScorer knnScorer = (KNNScorer) scorer(context); |
There was a problem hiding this comment.
why we are computing the scorer for every doc again and again? In case of explain should we just cache scorer in the KNNExplainObject once the first explain call happens.
There was a problem hiding this comment.
thats a good point, will make changes
yup, that WIP as my setup for ITs was broken. I am able to fix that now, however the PR has the coverage for all the UTs. |
Signed-off-by: Neetika Singhal <[email protected]>
Description
Add support for explain for Exact/ANN/Radial/Disk/Filtering k-nn search. Score calculation explanation is currently added only for ANN search.
Proposal for explain is given here: #875 (comment)
ITs - WIP..
Related Issues
Resolves #875
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.